Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support subplot creation at runtime via createGrid #45

Merged
merged 14 commits into from
Mar 3, 2019

Conversation

Vindaar
Copy link
Member

@Vindaar Vindaar commented Jan 31, 2019

This PR adds a Grid object and corresponding createGrid procedure, which can be used to create subplots using a grid at runtime. It's basically what was proposed in #43.

Plots can be assigned to the Grid object using []=. In addition more plots than defined by the numPlots argument of createGrid can be added by using add.
The given object is converted to PlotJson. A call to show on the grid, will calculate the required number of rows and columns (numPlotsPerRow is an optional argument to createGrid) and make a call to combine to get a valid PlotJson object of the subplot.

I had to move the implementation from plotly.nim to a submodule, to be able to call show from within plotly_subplots.

The Grid object has a layout field, which takes just a normal Layout object. This can also be given to createGrid, but can be set later if desired.

For some reason the show proc with filename arg for --threads:off does not compile. It acts as if the proc was called, even it isn't. Not sure what's up there.

I'm all ears for other ideas, features of different names for the args / objects.

@timotheecour let me know if this suits your needs. :)

`createGrid` returns a `Grid` object for which `[]`, `[]=` and `add`
are defined, so that the user may hand any `Plot[T]` object to the
`Grid`. Internally all plots are stored in a `seq[PlotJson]`.
# Note: internally the returned `Grid` object stores all plots already
# converted to `PlotJson` (i.e. the `layout` and `traces` fields are
# `JsonNodes`).
var grid = createGrid(numPlots = 2) #,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd prefer a rows, columns: int and a []/[]= that takes a row/column pair in order to better work with MxN subplots

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I personally agree with you. That's what I intuitively would expect when dealing with a 2D grid anyways. @timotheecour your opinion?
I'm fine either way though.

@brentp
Copy link
Collaborator

brentp commented Feb 4, 2019

I am fine with this. Seems like soon we need more general documentation now that the feature-set is expanding.

@Vindaar
Copy link
Member Author

Vindaar commented Feb 4, 2019

I am fine with this. Seems like soon we need more general documentation now that the feature-set is expanding.

Yes, I agree. I've been meaning to write some more documentation... :|

The result of this is we can have e.g. a 2x2 grid of plots and only
assign 3 without a crash. However, if we only assign (0, 0), (0, 1),
and (1, 1), the plot at (1, 1) will be shifted to (1, 0)!
Previously `numPlotsPerRow` caused the calculation of the # of rows
and columns to break, because we assumed `rows == 0` as a
placeholder.

Introduces `-1` as a special case for row or column, which means
"infer from the other dimension + nPlots".
Since `parseTraces` is also useful on the JS backend, it was in
addition added to `plotly_js`, since either putting it into
`plotly_sugar` or creating a separate file for JS + C specific
functions seemed unreasonable.
If the proc is exported its name should better reflect what it
actually does
@Vindaar
Copy link
Member Author

Vindaar commented Feb 18, 2019

Sorry for taking so long to get back to this.
I just:

  • added the option to assign subplots via (row, col) coordinate tuples
  • fixed a bug in the row / col calculation if numPlotsPerRow was set
  • renamed and exported showImpl to toPlotJson
  • fixed access to the JS backend
  • added some bounds checks when assigning plots

@LemonBoy: Does this look reasonable for you? :)

@brentp
Copy link
Collaborator

brentp commented Feb 28, 2019

I haven't had a look at this. @LemonBoy PTAL?

@Vindaar is this still as you'd like it?

@Vindaar
Copy link
Member Author

Vindaar commented Mar 1, 2019

For me this is fine. I think it's ok to support both ways of adding elements to the grid.
I've been using it in my own code already without issues.
edit: just noticed a problem with the JS backend. Not sure yet if it's a regression on devel or introduced by me in this PR. Will investigate and report back.
edit2: Sorry, done. I thought I tested the JS backend after making these changes, but apparently I didn't do that properly.

Vindaar added 2 commits March 1, 2019 13:26
`show` is only needed for the targets other than JS.
`hasThreadSupport` is defined here again to avoid having to import it
too from display.
@brentp brentp merged commit 3f1d4c1 into SciNim:master Mar 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants